Conversation
5200c68 to
b193c7a
Compare
f6e2a6c to
0842c19
Compare
There was a problem hiding this comment.
Pull request overview
Adds a feature-flagged “next-gen” logs table panel PoC, including persistence of table column widths and a new wrapText option stored in localStorage.
Changes:
- Introduces
LogsTablePanelScenethat renders thelogstableviz panel and hooks it intoLogsListScenebehindlogsTablePanelNG. - Persists/restores table column widths via new store helpers and a new
services/logsTablehelper module. - Extends feature flags + localStorage options to support
logsTablePanelNGandwrapText.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/store.ts | Adds localStorage persistence for table column widths; extends boolean log options with wrapText. |
| src/services/logsTable.ts | New helper for extracting/saving width overrides and reapplying them to the table builder. |
| src/featureFlags/openFeature.ts | Adds logsTablePanelNG flag and config fallback wiring (with a duplicated fallback branch). |
| src/Components/ServiceScene/LogsTablePanelScene.tsx | New Scene implementation for logstable panel; URL syncing/displayed field interactions and header actions wiring. |
| src/Components/ServiceScene/LogsListScene.tsx | Gates the new table panel behind logsTablePanelNG. |
| src/Components/ServiceScene/LogOptionsScene.tsx | Alters how the scene finds its “logs panel” parent (now using an any cast to this.parent). |
| project-words.txt | Adds logstable to dictionary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const searchParams = new URLSearchParams(locationService.getLocation().search); | ||
| let urlColumns: string[] | null = []; | ||
| try { | ||
| urlColumns = unknownToStrings(JSON.parse(decodeURIComponent(searchParams.get('urlColumns') ?? ''))); | ||
| // If body or line is in the url columns, show the line state controls | ||
| if (urlColumns.includes(DATAPLANE_BODY_NAME_LEGACY) || urlColumns.includes(DATAPLANE_LINE_NAME)) { | ||
| this.setState({ isDisabledLineState: true }); | ||
| } | ||
| } catch (e) { | ||
| console.error(e); | ||
| } |
There was a problem hiding this comment.
Same parsing issue as above: defaulting urlColumns to '' makes JSON.parse('') throw whenever the param is absent. Use a safe JSON default ('[]') or skip parsing when the param is null/empty; also avoid console.error here since logger is already available in this file.
| displayedFields: newDisplayedFields, | ||
| }); | ||
| // sync LocalStorage displayedFields for Go to explore | ||
| setDisplayedFieldsInStorage(this, parentModel.state.displayedFields); |
There was a problem hiding this comment.
After parentModel.setState({ displayedFields: newDisplayedFields }), local storage is updated using parentModel.state.displayedFields, which may still be the previous value (state updates are async). Persist newDisplayedFields directly (or update storage in a state subscription) to ensure the stored displayed fields match the new selection.
| setDisplayedFieldsInStorage(this, parentModel.state.displayedFields); | |
| setDisplayedFieldsInStorage(this, newDisplayedFields); |
| ? stored.filter( | ||
| (columnWidth: unknown): columnWidth is TableColumnWidth => | ||
| typeof columnWidth === 'object' && | ||
| columnWidth !== null && | ||
| 'field' in columnWidth && | ||
| 'width' in columnWidth && | ||
| typeof columnWidth.width === 'number' && | ||
| typeof columnWidth.field === 'string' | ||
| ) |
There was a problem hiding this comment.
The type guard in stored.filter(...) accesses columnWidth.width / columnWidth.field even though columnWidth is typed as unknown. In TypeScript this doesn’t narrow to a property-accessible shape, so this is likely a compile-time error. Consider first narrowing to a record (e.g., via an isRecord helper) or casting to Record<string, unknown> after the typeof === 'object' check before reading properties; then narrow width/field from that record.
| ? stored.filter( | |
| (columnWidth: unknown): columnWidth is TableColumnWidth => | |
| typeof columnWidth === 'object' && | |
| columnWidth !== null && | |
| 'field' in columnWidth && | |
| 'width' in columnWidth && | |
| typeof columnWidth.width === 'number' && | |
| typeof columnWidth.field === 'string' | |
| ) | |
| ? stored.filter((columnWidth: unknown): columnWidth is TableColumnWidth => { | |
| if (typeof columnWidth !== 'object' || columnWidth === null) { | |
| return false; | |
| } | |
| const record = columnWidth as Record<string, unknown>; | |
| return ( | |
| 'field' in record && | |
| 'width' in record && | |
| typeof record.width === 'number' && | |
| typeof record.field === 'string' | |
| ); | |
| }) |
| const widthOverrides = config.overrides | ||
| .filter((override) => override.matcher.id === 'byName') | ||
| .filter((override) => override.properties.some((property) => property.id === 'custom.width' && property.value > 0)) | ||
| .map((override) => { | ||
| const field = override.matcher.options; | ||
| const width = override.properties.find((property) => property.id === 'custom.width' && property.value > 0)?.value; | ||
| return { | ||
| field, | ||
| width, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
widthOverrides can end up storing entries where width is undefined (the find(...)? is still optional) and/or field isn’t a string (matcher options isn’t validated). This will either fail the TableColumnWidth contract or silently drop widths later when reloading. Prefer extracting the matching custom.width property once, assert typeof value === 'number' and value > 0, and only persist { field: string, width: number } entries.
| const widthOverrides = config.overrides | |
| .filter((override) => override.matcher.id === 'byName') | |
| .filter((override) => override.properties.some((property) => property.id === 'custom.width' && property.value > 0)) | |
| .map((override) => { | |
| const field = override.matcher.options; | |
| const width = override.properties.find((property) => property.id === 'custom.width' && property.value > 0)?.value; | |
| return { | |
| field, | |
| width, | |
| }; | |
| }); | |
| const widthOverrides = config.overrides.flatMap((override) => { | |
| if (override.matcher.id !== 'byName') { | |
| return []; | |
| } | |
| const field = override.matcher.options; | |
| const width = override.properties.find((property) => property.id === 'custom.width')?.value; | |
| if (typeof field !== 'string' || typeof width !== 'number' || width <= 0) { | |
| return []; | |
| } | |
| return [ | |
| { | |
| field, | |
| width, | |
| }, | |
| ]; | |
| }); |
| panel.setState({ | ||
| showMenuAlways: true, | ||
| menu: new PanelMenu({}), | ||
| headerActions: new LogOptionsScene({ | ||
| onChangeVisualizationType: parentScene.setVisualizationType, | ||
| visualizationType: parentScene.state.visualizationType, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
headerActions wires in LogOptionsScene, but LogOptionsScene reads sortOrder/wrapLogMessage from getLogsPanelScene() and mutates that scene’s state. In this context the parent is a VizPanel (not a LogsPanelScene), so this is likely to throw or mutate the wrong object at runtime. Consider providing table-appropriate header actions (or adapting LogOptionsScene to work with both logs and table panels via an explicit interface).
| export function storeTableFieldConfig(config: FieldConfigSource, sceneRef: SceneObject) { | ||
| const widthOverrides = config.overrides | ||
| .filter((override) => override.matcher.id === 'byName') | ||
| .filter((override) => override.properties.some((property) => property.id === 'custom.width' && property.value > 0)) | ||
| .map((override) => { | ||
| const field = override.matcher.options; | ||
| const width = override.properties.find((property) => property.id === 'custom.width' && property.value > 0)?.value; | ||
| return { | ||
| field, | ||
| width, | ||
| }; | ||
| }); | ||
| saveTableColumnWidths(sceneRef, widthOverrides); | ||
| } | ||
|
|
||
| export function setTableFieldOverrides( | ||
| builder: FieldConfigOverridesBuilder<LogsTableFieldConfig>, | ||
| sceneRef: SceneObject | ||
| ) { | ||
| const columnWidths = getTableColumnWidths(sceneRef); | ||
|
|
||
| columnWidths.forEach((columnWidth) => { | ||
| builder | ||
| .matchFieldsWithName(columnWidth.field) | ||
| .overrideCustomFieldConfig<LogsTableFieldConfig, 'width'>('width', columnWidth.width); | ||
| }); |
There was a problem hiding this comment.
New persistence behavior (extracting custom.width overrides and reapplying them via setTableFieldOverrides) isn’t covered by tests. Adding unit tests around storeTableFieldConfig/setTableFieldOverrides (including invalid/partial overrides and corrupted localStorage payloads) would help prevent regressions in column sizing persistence.
| if (flagName === 'exploreLogsShardSplitting') { | ||
| return config.featureToggles.exploreLogsShardSplitting; | ||
| } |
There was a problem hiding this comment.
getConfigToggleFallback checks flagName === 'exploreLogsShardSplitting' twice. The second branch is unreachable and should be removed to avoid confusion about intended fallback behavior.
| if (flagName === 'exploreLogsShardSplitting') { | |
| return config.featureToggles.exploreLogsShardSplitting; | |
| } |
| getLogsPanelScene = () => { | ||
| return sceneGraph.getAncestor(this, LogsPanelScene); | ||
| //@todo fix | ||
| return this.parent as any; | ||
| // return sceneGraph.getAncestor(this, LogsPanelScene); | ||
| }; |
There was a problem hiding this comment.
getLogsPanelScene currently returns this.parent as any. This is brittle and can easily break at runtime when LogOptionsScene is mounted under something other than LogsPanelScene (e.g., a VizPanel headerActions). Prefer resolving the correct scene via sceneGraph.getAncestor(...) (or pass an explicit reference/callbacks in state) so the renderer can reliably read/update sortOrder, wrapLogMessage, etc. without any casts.
| const searchParams = new URLSearchParams(locationService.getLocation().search); | ||
| // Check URL columns for body parameter and update isDisabledLineState accordingly | ||
| let urlColumns: string[] | null = []; | ||
| try { | ||
| urlColumns = unknownToStrings(JSON.parse(decodeURIComponent(searchParams.get('urlColumns') ?? ''))); | ||
|
|
||
| // If body or line is in the url columns, show the line state controls | ||
| if (urlColumns.includes(DATAPLANE_BODY_NAME_LEGACY) || urlColumns.includes(DATAPLANE_LINE_NAME)) { | ||
| this.setState({ isDisabledLineState: true }); | ||
| } else { | ||
| this.setState({ isDisabledLineState: false }); | ||
| } | ||
| } catch (e) { | ||
| console.error('Error parsing urlColumns:', e); | ||
| } |
There was a problem hiding this comment.
When urlColumns is missing/empty, JSON.parse(decodeURIComponent(searchParams.get('urlColumns') ?? '')) will always throw (JSON.parse('') is invalid), causing noisy errors on every navigation. Use a safe default like '[]' (or guard on null/empty) before parsing, and prefer the shared logger instead of console.error for consistency/telemetry.
No description provided.